Show "Custom" section in "Get Started" screen #3523
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| it('should return checkpoint plots with empty values if there no selected revisions', () => { | ||
| const expectedOutput: CustomPlotData[] = customPlotsFixture.plots.map( | ||
| plot => | ||
| plot.type === CustomPlotType.CHECKPOINT ? { ...plot, values: [] } : plot |
There was a problem hiding this comment.
This feels odd:
I don't know
- Which plot is which
- What type of plot should be inside (trends or metrics vs params).
If there are no experiments at all then we end up with this:
Seems inconsistent.
At the very least I think the text inside of the empty box needs to mention the plot title/select a checkpoint experiment to display but I think they should be hidden with some message to say "select a checkpoint experiment to display trend plots"
Note: Chances of users seeing these screens are minimal due to the use of checkpoint experiments but I think we should attempt to get it right.
There was a problem hiding this comment.
Also, need to consider when there are no custom plots added but we do have data available:
An update will definitely be needed after #3567. Probably want to add a button.
There was a problem hiding this comment.
At the very least I think the text inside of the empty box needs to mention the plot title/select a checkpoint experiment to display but I think they should be hidden with some message to say "select a checkpoint experiment to display trend plots"
A message makes sense! Not sure where it could go though 🤔. Will start experimenting with some ideas :)
Also, need to consider when there are no custom plots added but we do have data available:
What's the issue with the current setup? Text says "No Plots Added" and you can add a plot with the plus button 🤔
There was a problem hiding this comment.
What's the issue with the current setup? Text says "No Plots Added" and you can add a plot with the plus button
The screen shouldn't be split. The custom section should be hidden and there should be two buttons, one to add an experiment and one to add a custom plot.
There was a problem hiding this comment.
There are 3 things that can currently cause an "empty" plots screen:
- No plots in the project
- No plots selected
- No experiments selected
I'm about to introduce a global error too which gives 4.
I think we need to distinguish between the user having added custom plots or not.
From those we end up with the following combinations:
| project has plots | plots selected | experiments selected | project has custom plots | project has global error | welcome screen content | buttons to add/create | show or hide custom plots section |
|---|---|---|---|---|---|---|---|
| N/A | N/A | N/A | N | Y | Error message | custom plots | hide |
| N | N/A | N/A | N | N | Info on adding plots to the project | custom plot | hide |
| Y | N | N | N | N | There are no selected plots or experiments | plots, experiments, custom plots | hide |
| Y | N | Y | N | N | There are no selected plots or custom plots | plots, custom plots | hide |
| Y | Y | N | N | N | The are no selected experiments or custom plots | experiments, custom plots | hide |
| N/A | N/A | N/A | Y | Y | Error message | custom plots | show |
| N | N/A | N/A | Y | N | Info on adding plots to the project | none | show |
| Y | N | N | Y | N | There are no selected plots or experiments | plots, experiments | show |
| Y | N | Y | Y | N | There are no selected plots | plots | show |
| Y | Y | N | Y | N | There are no experiments selected | experiments | show |
| Y | Y | Y | Y | N | Normal webview | N/A | show |
I added suggested welcome screen content, buttons and whether I think we should show or hide the custom plots section. IMO the section should be hidden if there is no other data sent the webview and a button should be added to create new custom plots.
I even think we should do this when there is a global CLI error.
The alternative is that we throw away the welcome screen concept in favour of improving the empty state of the existing sections. We then only have to deal with the project having no data at all.
There was a problem hiding this comment.
Another alternative is to simplify the welcome screen into as few states as possible.
| project has plots | plots selected | experiments selected | project has custom plots | project has global error | welcome screen content | buttons to select/create | show or hide custom plots section |
|---|---|---|---|---|---|---|---|
| N | N/A | N/A | N | N | Info on adding plots to the project | custom plot | hide |
| Y | N/A | N/A | N | N | There are no plots to show | experiments, plots, custom plots | hide |
| N | N/A | N/A | Y | N | Info on adding plots to the project | none | show |
| Y | N/A | N/A | Y | N | There are no plots to show | experiments, plots, experiments | show |
We can deal with global errors separately but that would give us 6 states instead of 11. Seems pretty reasonable to me.
There was a problem hiding this comment.
Thanks for the clarification! Updated the logic.
webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx
Outdated
Show resolved
Hide resolved
|
@julieg18 let's put this message after the existing plots and make it similar to the regular plots section. We can say something "Some plots require experiments to be selected. Add an experiment (can be a button)". We can also show a second button "+ Add" (to add a custom plot). |
|
As discussed in the planning meeting, we're going to drop checkpoints, removing trends checkpoint plot types eventually. I'll go ahead and remove the extra logic/ui surrounding the message when there are no selected experiments in this pr! |
There was a problem hiding this comment.
Resolved #3523 (comment) and ready for another round of reviews.
| @@ -283,17 +283,68 @@ describe('App', () => { | |||
| expect(loading).toHaveLength(3) | |||
| }) | |||
|
|
|||
| it('should render the Add Plots and Add Experiments get started button when there are experiments which have plots that are all unselected', async () => { | |||
There was a problem hiding this comment.
Tried my best to cover all the possibilities for the get started screen and to make the rather lengthy text clear 😅
|
Code Climate has analyzed commit 39b349a and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.0% (0.0% change). View more on Code Climate. |
Demo
https://user-images.githubusercontent.com/43496356/228010939-427af9d3-cd2a-41f5-ac87-cb77c107a006.movhttps://user-images.githubusercontent.com/43496356/228312421-5275a067-87cb-48e4-bdf8-5480c8bc2ad7.movScreen.Recording.2023-04-03.at.9.36.17.AM.mov
Part of #3373